Skip to content

Syntax operations also need to ensure project is present for the open script infos since update could be pending to make sure open script info has project - #50418

Merged
Sheetal Nandi (sheetalkamat) merged 1 commit into
mainfrom
noProject
Aug 24, 2022

Conversation

@sheetalkamat

Copy link
Copy Markdown
Member

Also convert one relevant test case to baseline
Fixes #50131

… script infos since update could be pending to make sure open script info has project

Also convert one relevant test case to baseline
Fixes #50131

@amcasey Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a project be required for outlining spans? Are there project-scoped settings that affect the behavior? Same question for (e.g.) brace matching.

Comment thread src/server/session.ts
@DanielRosenwasser

Copy link
Copy Markdown
Member

So what exactly happened before? Was it something like this?

  1. Open a file - a default project is automatically made.
  2. Close the file - the default project is automatically closed because it has no files open.
  3. Open another file - the default project is assumed not to be closed (?)

@sheetalkamat

Copy link
Copy Markdown
Member Author

So what exactly happened before?

  • Open a file say src/file.ts. Opens configured project say tsconfig.json
  • Close this file. Configured project is not collected because the removal of projects and scriptinfos that are orphan happens only when opening file
  • open another file say tests/file.ts It finds a tsconfig file tests/tsconfig.json but that does not include this file. But existing tsconfig.json has this file so we dont create inferred project. After this collecting orphan project collects tsconfig.json because there is no open file which will open this project (tests/file.ts does not count because any file in tests folder would open tests/tsconfig.json and not tsconfig.json) So now tests/file.ts is not part of any project
  • Call outlinging span and we were throwin error before the fix. With the fix it sees there is no project for this file so it creates inferred project for this file. (just like it would have if we didnt do first two steps)

@amcasey

Copy link
Copy Markdown
Member

I'm still missing why we'd need a project at all to handle this request. Is that just an unfortunate consequence of our design?

Comment on lines +1614 to +1621
const testsConfig: File = {
path: `${tscWatch.projectRoot}/playground/tsconfig.json`,
content: "{}"
};
const testsFile: File = {
path: `${tscWatch.projectRoot}/playground/tests.ts`,
content: `export function foo() {}`
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a nit - but really, is the idea that that playground contains some sort of implementation file, and playground/tests would contain the test file? In theory, you should only need 2 tsconfigs and 2 .ts files - but you don't need innerSrcFile, right?

src/
├── tsconfig.json
├── foo.ts
└── spec/
    ├── tsconfig.json
    └── foo.ts

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replication of repro at #50131 (comment)

The playground/tsconfig.json is tests config i think or atleast the file opened was test file which imported the inner test file
The innerConfig is the implementation configuration but the test file resides next to it and isnt part of the config

Definitely not ideal configuration for sure. But the PR ensures that we dont crash and project is assigned.
We never look past first tsconfig.json we find because otherwise we would risk loading too many projects when opening file.

@sheetalkamat

Copy link
Copy Markdown
Member Author

I'm still missing why we'd need a project at all to handle this request. Is that just an unfortunate consequence of our design?

Thats our design for now.. in most cases the open files are already part of the project (the scenario we are fixing is just unfortunate) so it was never priority to experiment separate project for syntax only operations. (To get hold of LS on which we can forward those requests)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No Project error during getOutliningSpans

4 participants